fix: decode invalid UTF-8 at the JVM to native FFI import boundary - #5310
Conversation
There was a problem hiding this comment.
Thanks @manuzhang for picking this up. left some comments
|
Thanks for picking this up @manuzhang. I probably can't review in detail until next week. How does this relate to the work in #5267? Will that PR remain open? |
|
@andygrove #5267 has been closed in favor of this approach. |
e81cf0d to
d751c60
Compare
This is worth doing. Three things.
Is that path reachable with string data? If it is, it needs The default arm silently passes through _ => Ok(Arc::clone(array)),The comment is honest about the risk and says a view-typed column would silently return the UB. But a comment is not a mechanism. In a function whose entire purpose is to prevent unsoundness, an unknown type should not take the quiet path. Could the default arm return an error for the types that could plausibly contain strings, specifically The performance cost needs an end-to-end number Every string column crossing the JVM to native boundary now pays a full UTF-8 validation pass per batch. The benchmark measures Could you add a before-and-after on something string-heavy end to end, say TPC-H Q1 or a filter over a wide string column with One smaller note
|
ed0d2f2 to
cf1e794
Compare
|
@andygrove could you please take another look? |
cf1e794 to
c644262
Compare
c644262 to
fadc7f2
Compare
|
Hi @manuzhang |
Co-authored-by: Manu Zhang <owenzhang1990@gmail.com> Co-authored-by: Codex <codex@openai.com>
Measures the UTF-8 validation that native code now runs on every string column imported from the JVM, at the two import sites that carry most string data: `ScanExec` fed by a JVM operator (Spark's Parquet reader converted to Arrow) and native columnar-to-row conversion. Each imported case is paired with a control that does the same work without an import, and every Comet case checks its executed plan before it is timed. Both ASCII and multibyte wide strings are covered, since validation takes a slower path on non-ASCII bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mypwig1QwDcPvDMadVB1d
`decode_string_arrays` validated every imported string column with `std::str::from_utf8`. That has a fast ASCII path but falls back to a byte-at-a-time loop on non-ASCII text: 1.65 GB/s, or 111 ns for a 184-byte row. Measured end to end on wide multibyte strings, importing through a JVM Parquet scan was 2.1x slower and through native columnar-to-row 3.3x slower, against 6-11% for ASCII. simdutf8 validates the same buffers at 11.7 GB/s (15.7 ns/row), 7x faster on multibyte data and 3x on ASCII. It is already in the dependency graph through arrow-json, so this adds no new third-party crate. The `basic` validator is enough here because an invalid buffer falls through to the element-by-element decoder below, which finds the bad bytes itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mypwig1QwDcPvDMadVB1d
fadc7f2 to
62aafe7
Compare
andygrove
left a comment
There was a problem hiding this comment.
All three points from my last review are addressed, and I checked the soundness-critical part rather than taking it on trust.
Validating values[start..end] and then rejecting any interior offset that lands on a continuation byte is sufficient. In a valid buffer a non-continuation position is a codepoint boundary, and splitting valid UTF-8 at codepoint boundaries leaves every element valid. offsets[0] and offsets[len] do not need their own check, because a leading or trailing split would already fail the range validation. The o < values.len() guard can only cost a trip through the slow path on a sliced array, never a missed split.
Listing all four from_ffi sites and recording why batch_from_ffi is exempt is what I was after. Making data_type_contains_string exhaustive and returning NotYetImplemented for the view types turns the comment into a mechanism, which was the point.
One thing is still open, and it is the one you flagged yourself. The end-to-end table is the std::str::from_utf8 build and the simdutf8 figures are a projection from the validator microbenchmark. That is honest and I do not think it should hold the fix, since this closes undefined behaviour in the default configuration. Could you re-run CometStringFfiImportBenchmark on the EC2 micro-benchmark runner after this lands and update the table, so the numbers users see are measured rather than extrapolated?
Approving.
This PR continues the work from #4945.
Which issue does this PR close?
Part of #4764 (EPIC: consistent handling of invalid UTF-8 in native StringType). This PR implements Gap B only, decoding invalid UTF-8 at the JVM to native Arrow FFI import boundary. It does not close the EPIC, which still tracks Gap A (the native scan rejecting invalid UTF-8).
Rationale for this change
Spark's
StringType(UTF8String) can hold arbitrary bytes, including sequences that are not valid UTF-8. When a JVM side source hands string columns to native code over the Arrow C Data Interface, arrow-rs imports them withfrom_ffi/from_ffi_and_data_type, which build the array viaArrayData::new_uncheckedand do not validate UTF-8. The imported ArrowUtf8/LargeUtf8array then lies about its validity, and any downstream native string kernel that reads&strthrough arrow-rs's uncheckedStringArray::value()(from_utf8_unchecked) exercises undefined behaviour: iterating chars, slicing on char boundaries, and similar operations can misbehave, panic, or be miscompiled. This is a latent, default configuration soundness hazard.The string producing sites already decode invalid bytes the way Spark renders them:
CAST(binary AS string)(#4763) and native shuffleget_string(#4521) both usedecode_utf8_spark_lossy. This PR applies the same policy to the string ingress side, so the whole native pipeline agrees on a single invariant: native string data is always valid UTF-8.What changes are included in this PR?
A new
decode_string_arrayswalker indatafusion-comet-common, beside the existingdecode_utf8_spark_lossy. It ensures everyUtf8/LargeUtf8array reachable from an imported column holds valid UTF-8, decoding invalid bytes to Spark's rendered form. It is zero copy for the valid common case (one validation pass plus an O(number of strings) boundary check, returning the sameArc), and rebuilds element by element only when bytes are genuinely invalid. It recurses throughDictionary,Struct,List,LargeList,FixedSizeList, andMap.There are four
from_fficall sites. Three carry string data and decode directly:ScanExec::pull_next, which handles all native query input (the JVM reader, Spark columnar handoff, shuffle reads, mapInArrow). Decoding runs before dictionary unpack so compact dictionary values are validated rather than the expanded ones.columnarToRowconversion.The fourth,
batch_from_ffiinaligned_stream_reader.rs, is consumed only byScanExec, which passes every column throughimport_columnimmediately after the batch is materialised. Decoding there as well would validate the same buffers twice, so its doc comment records that any future consumer must keep that step.Anything string-bearing that the walker does not handle fails closed rather than passing unchecked data through.
data_type_contains_stringis an exhaustive match, so an Arrow upgrade that adds a data type forces this decision to be revisited, and types such asUtf8View, the view lists andRunEndEncodedreturnNotYetImplementedif they ever reach this boundary.The fast path validates the used byte range and also confirms no element boundary splits a codepoint. This second check is required for soundness: a whole buffer valid
"é"(bytesC3 A9) split across two offsets would otherwise hand each element an invalid slice, andvalue()decodes those unchecked.Validation uses
simdutf8rather thanstd::str::from_utf8(see Performance below).simdutf8is already in the dependency graph througharrow-json, so this adds no new third-party crate.Benchmarks: a criterion benchmark for the valid fast path, and
CometStringFfiImportBenchmark, which measures both import sites end to end.No configuration flag gates this. It fixes undefined behaviour in the default configuration and matches the always on behaviour of the sibling cast and shuffle fixes.
The only observable divergence from Spark is the previously documented one: decoding rather than preserving raw bytes differs only under byte level round trips (for example
CAST(CAST(X'FF' AS STRING) AS BINARY)), already noted in the compatibility guide.Performance
Validation costs one pass over each imported string buffer, so the cost scales with the bytes crossing the boundary, not with the number of rows.
CometStringFfiImportBenchmark(added here) measures both import sites over 1M rows of roughly 180 byte strings, in ASCII and in multibyte text. Each imported case is paired with a control that does the same work without crossing an import boundary, and every case checks its executed plan before it is timed. Best time on an M2:The controls stay flat, which is what places the cost in validation rather than anywhere else.
Those numbers come from validating with
std::str::from_utf8. It has a fast ASCII path but degrades to a byte at a time loop on non-ASCII text. Timing the validator alone on the same string shapes, 8192 rows per batch:std::str::from_utf8simdutf8::basic111 ns/row accounts for the whole multibyte regression above, and the boundary check costs under 1 ns/row. The last commit therefore switches the fast path to
simdutf8, which is 7x faster on multibyte data and 3x on ASCII. Substituting 15.7 ns/row back into the cases above projects roughly +15% for the multibyte converted scan and +29% for multibyte native columnar to row, with ASCII near noise.The
simdutf8numbers are measured at the validator, not end to end. Repeated attempts to re-time the full benchmark against thesimdutf8build on this machine were spoiled by background load (the same case varied up to 4x between runs), so the end to end table above is still thestdbuild. A quiet machine or the EC2 micro benchmark runner (benchmarks/micro/run.py) should confirm the projection before this is taken as final.Skipping validation where the producer is known to be native is not available as a shortcut: native columnar to row can receive strings built on the JVM, for example through a union, the in-memory cache, or broadcast.
How are these changes tested?
Dictionary/Struct/List/FixedSizeList/Map(decode and zero copy paths); null preservation; sliced arrays with a non zero starting offset; and trailing empty strings.ScanExecimport site proving an invalid UTF-8 column is decoded through the production per column path.simdutf8fast path.CometStringFfiImportBenchmarkfor the two import sites end to end.